Gh 716 push notifications #730

sche · 2020-11-25T16:32:18Z

No description provided.

codecov · 2020-11-25T16:35:16Z

Codecov Report

Merging #730 (72ff4e9) into main (4c71fd8) will increase coverage by 1.06%.
The diff coverage is 63.30%.

@@            Coverage Diff             @@
##             main     #730      +/-   ##
==========================================
+ Coverage   28.78%   29.85%   +1.06%     
==========================================
  Files         324      328       +4     
  Lines        9262     9370     +108     
==========================================
+ Hits         2666     2797     +131     
+ Misses       6596     6573      -23

Impacted Files	Coverage Δ
...e Transaction Service/SignTransactionRequest.swift	`0.00% <0.00%> (ø)`
Multisig/Logic/Models/SafeTransactionSigner.swift	`38.46% <0.00%> (+13.46%)`	⬆️
...ingsViewController/AppSettingsViewController.swift	`0.00% <0.00%> (ø)`
...ingsViewController/ContractVersionStatusCell.swift	`0.00% <0.00%> (ø)`
...ngsViewController/SafeSettingsViewController.swift	`0.00% <0.00%> (ø)`
.../UI Library/ReusableCells/AddressDetailsCell.swift	`0.00% <0.00%> (ø)`
...I Library/ReusableViews/AddressWithTitleView.swift	`0.00% <ø> (ø)`
.../Owner Wallet Management/EnterSeedPhraseView.swift	`0.00% <0.00%> (ø)`
...allet Management/SelectOwnerAddressViewModel.swift	`0.00% <0.00%> (ø)`
Multisig/UI/App/RemoteNotificationHandler.swift	`34.83% <25.00%> (+3.05%)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72e2baf...72ff4e9. Read the comment docs.

DmitryBespalov

I have several questions that need resolution

Multisig/UI/App/RemoteNotificationHandler.swift

Multisig/Logic/Models/Signer.swift

Multisig/Data/Services/Notification Service/RegisterNotificationTokenRequest.swift

...isigTests/Data/Services/Safe Transaction Service/RegisterNotificationTokenRequestTests.swift

DmitryBespalov · 2020-11-30T15:12:18Z

Multisig/Data/Services/Notification Service/RegisterNotificationTokenRequest.swift

        ]
        .joined()

        if let signature = try? Signer.sign(string).value {
+            guard timestamp != nil else {
+                throw "'timestamp' parameter is required if signing key exists"


maybe preconditionFailure() is better because this would be a programmer's error

DmitryBespalov · 2020-11-30T15:44:30Z

Multisig/Data/Services/Notification Service/UnregisterNotificationTokenRequest.swift

        self.address = address.checksummed
-        self.deviceID = deviceID.uuidString.lowercased()
+        self.deviceID = deviceID


the lowercased() got lost - either when this init is called or in here it should be present

I checked - it worked fine. But I will make it lowercased()

DmitryBespalov · 2020-11-30T15:46:05Z

Multisig/UI/App/UserDefaultPropertyWrapper.swift

@@ -17,6 +17,16 @@ struct UserDefault<T> {
    }
 }

+@propertyWrapper
+struct UserDefaultWithDefault<T> {


not sure if we need this anymore

DmitryBespalov · 2020-11-30T15:47:48Z

Multisig/Data/Services/Notification Service/RegisterNotificationTokenRequest.swift

+         timestamp: String?) throws {
+
+        guard UUID(uuidString: deviceID) != nil else {
+            throw "'deviceID' should be UUID string"


I would make it preconditionFailure() or just precondition instead of guard because this is developer error

DmitryBespalov

thank you! 👏

Andrey Scherbovich added 2 commits November 25, 2020 15:23

gh-716 update RegisterNotificationTokenRequest

8c9350d

gh-716 handle CONFIRMATION_REQUEST pushes; add unit tests

051838b

sche added this to the Sprint 17 "..." milestone Nov 25, 2020

sche requested review from DmitryBespalov and MouazAlzahabi November 25, 2020 16:32

sche self-assigned this Nov 25, 2020

DmitryBespalov reviewed Nov 26, 2020

View reviewed changes

Andrey Scherbovich added 3 commits November 26, 2020 15:03

gh-716 make adjustments

2175b72

gh-716 add one more test

786877f

gh-716 make timestamp optional

d5a2105

DmitryBespalov reviewed Nov 30, 2020

View reviewed changes

gh-716 improvements

5583f6f

DmitryBespalov reviewed Nov 30, 2020

View reviewed changes

gh-716 improvements

6c73ebe

DmitryBespalov approved these changes Nov 30, 2020

View reviewed changes

gh-716 fix tests

72ff4e9

sche merged commit 3a2752e into main Nov 30, 2020

sche deleted the gh-716-push-notifications branch November 30, 2020 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gh 716 push notifications #730

Gh 716 push notifications #730

sche commented Nov 25, 2020

codecov bot commented Nov 25, 2020 •

edited

DmitryBespalov left a comment

DmitryBespalov Nov 30, 2020

sche Nov 30, 2020

DmitryBespalov Nov 30, 2020

sche Nov 30, 2020

DmitryBespalov Nov 30, 2020

sche Nov 30, 2020

DmitryBespalov Nov 30, 2020

sche Nov 30, 2020

DmitryBespalov left a comment

Gh 716 push notifications #730

Gh 716 push notifications #730

Conversation

sche commented Nov 25, 2020

codecov bot commented Nov 25, 2020 • edited

Codecov Report

DmitryBespalov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitryBespalov left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 25, 2020 •

edited